fix(cli): Open a session without a default schema - #1825
Conversation
|
@mbasmanova Could you help take a look at this PR? Thank you. I only have read access to this repository, so I’m unable to add reviewers directly. |
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you for the fix.
--query reports success when a statement fails. Console::runOnce catches the error, prints Query failed: ... and returns false; runMultiple stops; run() is void, so main falls through to return 0. This predates the change and holds for every catalog — --query "select * from nonexistent" against tpch exits 0 today — but the change routes one more failure into it. A bare table name against a schemaless catalog used to abort during runner.initialize() with a non-zero exit; now it is an ordinary failed statement that a script cannot detect. runOnce already returns the bool; passing it up through runMultiple and run() would let main exit non-zero.
The hint prints in non-interactive runs. Console::run gates its greeting on interactive and gates live progress on isatty(STDERR_FILENO), with the comment that the non-interactive paths opt in via --show_live_progress — the convention being that --query and piped stdin emit query output and nothing else. The new message is printed from main before any of that is known, so every scripted run against a schemaless catalog carries it. Could it move to where the mode is known and follow the greeting?
|
@mbasmanova Thank you for the review. Addressed the comments by:
Fixed by passing the return code to main(), and this introduce some behavior change. Listed in the PR description.
Moved the hint to inside the existing |
mbasmanova
left a comment
There was a problem hiding this comment.
Thank you — the exit-code table in the description and the scrut cases make this easy to check.
One behaviour I want to make sure is intended: a failed --init statement now returns before the interactive branch, so the prompt never appears. For a scripted run that is clearly right. For an interactive one the user has already seen Query failed: ..., and --init can be expensive — generating data, creating tables — so exiting costs them the whole setup rather than dropping them at a prompt where they can look around and fix it. Would reporting the failure and still opening the REPL be better there?
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this in D119443445. |
|
Thank you for making this change. |
|
@mbasmanova @singcha Thank you for the comments. I see the PR has been imported now. Can I amend the commit to fix these issues? If not, I can open a follow-up PR after this one is merged. |
Only three connector names carry a built-in default schema, a catalog with no default could not be opened at all. The session already sets a schema at any point with
use <catalog>.<schema>, and a qualified table name needs none.With this patch, the missing default now leaves the schema empty and prints what to run instead of refusing to start.
And a session now opens with an empty default schema. Only bare table names need one, so tables are reached either by qualifying them or by setting a schema for the session:
The REPL prints a one-line reminder of that after its greeting. Scripted runs stay quiet.
Two changes carry it:
trythat printsError: ..., so a startup problem exits 1 instead of aborting.Console::run()returns whether the run succeeded, so a failed statement reachesmain's exit code rather than being swallowed.These two changes the exit codes:
Startup, before any session open
--data_path+--etc_dirError: … mutually exclusive--catalog nosuchError: Catalog does not exist: nosuch--catalog system(no default schema)--repeat 0,--repeatwith no SQL, unreadable--initNon-interactive —
--query, or piped stdin--repeat N, a run failsInteractive REPL — nothing changed
.exit/.quit/ Ctrl+D--init— runs in both modes--initstatement fails, then--query--querystill ran--queryskipped--initstatement fails, interactive